-
-
Notifications
You must be signed in to change notification settings - Fork 101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce Table of Content #1201
base: main
Are you sure you want to change the base?
Conversation
a24186a
to
f3b5022
Compare
@ShaopengLin The feature work well to my opinion, but we need to change/polish the way how the TOC is displayed, see my comment in the issue. On the top:
|
@kelson42 To clarify, what does the view entry do? I didn't see it from the mockup, but I assume you mean either a button to collapse the entire TOC, or it will do the same thing as the TOC icon. Like a show/hide button? |
Not sure to fully understand you, but "yes", this is about toggling (hide/show) the TOC sidebar. |
768f3ed
to
a25474d
Compare
@kelson42 Improved the TOC as much as possible requested in this comment #1201 (comment). I will refactor the code again once the UI is ready. I haven't been able to find a way to inject custom fonts with javascript, so currently it is in Arial. I am looking into some ways to make this work. |
a25474d
to
ea50658
Compare
@ShaopengLin Sorry, I don't get it, shortcut does not display the TOC sidebar AND no icon in the toolbar |
ea50658
to
ddb58d1
Compare
@kelson42 I didn't find any issues on my end. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works now!
- If in library or no content displayed, then toolbar button should be greyed (like random button I guess)
- I believe that It should not be possible to display the toc sidebar at the same time like the bookmark sidebar. Displaying one should hide the other
- Secure look an feel is the same between bookmark and toc sidebar (The one of the TOC sidebar is better, it should be the reference)
- Loading a new article should not hide the TOC sidebar
- The width of the sidebar is a bit uselessly big, margin left and right should be smaller..
- Strategy should be clear if a section title is longer than the width: ellipsis en full title in the tooltip (when the mouse is over)
- I believe it would be clearer if the full background is highlighed when the mouse goes over the items. For the moment we can not keep the selection I guess as it should then move dynamically when we scroll the article
@kelson42 Most are doable.
I will try my best on the ellipsis (Had some trouble with it on the CSS side). The tooltip should be doable.
Out of scope for this PR, I will open an issue. The reading list's UI refactoring is long overdue anyway...
Do you mean to highlight the items in the TOC when we hover them? Not sure what "full background" means here. The dynamic scrolling might be too much for this PR... an indicator of localization has proven rather hard for me given the time constraint. I will open an issue afterward of this. |
I believe you understand properly. Item background highlighted from the left to the right of the sidebar
Indeed. Thx |
ddb58d1
to
491f29a
Compare
@kelson42 All issues mentioned are resolved:
Once we finally agree on the UI I can refactor the code. The implementation is hard and due to time constraints, I didn't do this methodically. I will ping veloman when I finish by then. |
491f29a
to
6206e52
Compare
@kelson42 Disable behavior now matches the random button. The rest of the UI request is fixed. |
@ShaopengLin Not all points: On the following screenshot you clear see:
For the rest it looks good and when these two points are sorted out then we can probably start with code review |
f2b0b6b
to
6f94ad7
Compare
@kelson42 Fix size done.
This isn't intentional. If you highlight any text, that's just how they are highlighted. |
@veloman-yunkan One question, am I allowed to introduce custom URL paths like "zim://kiwix-desktop-toc" ? Since we are using injected javascript, loading style sheet, icon, font, etc. would be much easier if we provide a request API path. It also is useful to pass information from the website to our application from some custom event. |
I hope with what follows, what is left to do is clearer: STILL A PROBLEM: If I have ctrl+B + ctrl+R + Ctrl+M and then again ctrl+R... every two attempts the TOC disappear after loading a random page. |
4c1f826
to
97e595a
Compare
@veloman-yunkan I will tag you when the code is ready. |
@ShaopengLin Still a bug: If section title is shorten, then there is not hint displayed with the full title! |
97e595a
to
bf6b908
Compare
@kelson42 I could not reproduce this on my own ZIM files. Could you direct me to where your example file is? |
bf6b908
to
86f30fa
Compare
There is not tooltip on your screenshot either! I'm not talking about the tooltip at the bottom which display the targeted URL, I'm talking about a tooltip displyed at the section title displaying the title of the section (you have it already because you display it in the TOC). |
86f30fa
to
f992132
Compare
@kelson42 Done, see if it matches what you are looking for stylistically. |
Yes, I would not have positioned the tooltip like this, but I like the way how you have fixed it! |
f992132
to
24b95cc
Compare
@ShaopengLin Any news abou the code cleaning you are running? It's important we start soon with code review. |
@kelson42 I have just finished the zim search one today as its a large change. I will start on this shortly. This one should take slightly less time. |
24b95cc
to
e2ccc2a
Compare
@veloman-yunkan Code is ready for review. |
Setup for TOC Javascript/CSS Injection
Same width as reading list.
Pass TOC title translation. More to come later
Show/Hide TOC with button and shortcut
Extra button for user to close TOC
Two side bar will be mutually exclusive.
Style title and hide button text
Retain the original look of the headers
Fix #42
Changes: